chore: refactor e2e test diagnostic#1144
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (11)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThe PR updates e2e failure diagnostics in the framework and test callsites. Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: simonpasquier The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/uiplugin_cluster_health_analyzer_test.go`:
- Around line 34-40: Register the UIPlugin and namespace failure diagnostics
before the first potentially failing assertion in the test so a create failure
still captures them: in the test that calls resetMonitoringUIPlugin and
f.K8sClient.Create, move f.DumpOnFailure(t, f.DebugUIPlugin(plugin.Name)) ahead
of the create/assertion, and add failure dumping for prometheusRuleNamespace as
well since the later PrometheusRule setup and wait checks depend on it. Keep the
existing uiPluginInstallNS dump, but ensure both namespaces and the plugin dump
are registered early enough to cover all failure paths in this test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 8fb8f16a-552e-4378-b48e-08856be3bc7d
📒 Files selected for processing (10)
test/e2e/framework/framework.gotest/e2e/framework/uiplugin.gotest/e2e/monitoring_stack_controller_test.gotest/e2e/observability_installer_test.gotest/e2e/operator_metrics_test.gotest/e2e/po_admission_webhook_test.gotest/e2e/prometheus_operator_test.gotest/e2e/thanos_querier_controller_test.gotest/e2e/uiplugin_cluster_health_analyzer_test.gotest/e2e/uiplugin_test.go
| f.DumpOnFailure(t, f.DebugNamespaces(uiPluginInstallNS)) | ||
|
|
||
| plugin := resetMonitoringUIPlugin(t) | ||
| err = f.K8sClient.Create(t.Context(), plugin) | ||
| assert.NilError(t, err, "failed to create monitoring UIPlugin") | ||
|
|
||
| f.DumpOnFailure(t, dumpUIPluginDebug(plugin.Name)) | ||
| f.DumpOnFailure(t, f.DebugUIPlugin(plugin.Name)) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Register all relevant failure diagnostics before the first assertion can fail.
plugin.Name is known before the create, so f.DebugUIPlugin(plugin.Name) should be registered before Line 37; otherwise a create failure skips the new UIPlugin dump entirely. Also, this test later creates the PrometheusRule in prometheusRuleNamespace, but the namespace dump only covers uiPluginInstallNS, so failures in the alert/incident waits miss the rule namespace events and objects.
Suggested fix
- f.DumpOnFailure(t, f.DebugNamespaces(uiPluginInstallNS))
+ f.DumpOnFailure(t, f.DebugNamespaces(uiPluginInstallNS, prometheusRuleNamespace))
plugin := resetMonitoringUIPlugin(t)
+ f.DumpOnFailure(t, f.DebugUIPlugin(plugin.Name))
err = f.K8sClient.Create(t.Context(), plugin)
assert.NilError(t, err, "failed to create monitoring UIPlugin")
-
- f.DumpOnFailure(t, f.DebugUIPlugin(plugin.Name))As per path instructions, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| f.DumpOnFailure(t, f.DebugNamespaces(uiPluginInstallNS)) | |
| plugin := resetMonitoringUIPlugin(t) | |
| err = f.K8sClient.Create(t.Context(), plugin) | |
| assert.NilError(t, err, "failed to create monitoring UIPlugin") | |
| f.DumpOnFailure(t, dumpUIPluginDebug(plugin.Name)) | |
| f.DumpOnFailure(t, f.DebugUIPlugin(plugin.Name)) | |
| f.DumpOnFailure(t, f.DebugNamespaces(uiPluginInstallNS, prometheusRuleNamespace)) | |
| plugin := resetMonitoringUIPlugin(t) | |
| f.DumpOnFailure(t, f.DebugUIPlugin(plugin.Name)) | |
| err = f.K8sClient.Create(t.Context(), plugin) | |
| assert.NilError(t, err, "failed to create monitoring UIPlugin") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/uiplugin_cluster_health_analyzer_test.go` around lines 34 - 40,
Register the UIPlugin and namespace failure diagnostics before the first
potentially failing assertion in the test so a create failure still captures
them: in the test that calls resetMonitoringUIPlugin and f.K8sClient.Create,
move f.DumpOnFailure(t, f.DebugUIPlugin(plugin.Name)) ahead of the
create/assertion, and add failure dumping for prometheusRuleNamespace as well
since the later PrometheusRule setup and wait checks depend on it. Keep the
existing uiPluginInstallNS dump, but ensure both namespaces and the plugin dump
are registered early enough to cover all failure paths in this test.
Source: Path instructions
There was a problem hiding this comment.
there's no need to dump the plugin data if it wasn't created.
simonpasquier
force-pushed
the
refactor-e2e-test-diagnostic
branch
from
54342f2 to
9a23f8c
Compare
Signed-off-by: Simon Pasquier <spasquie@redhat.com>
simonpasquier
force-pushed
the
refactor-e2e-test-diagnostic
branch
from
9a23f8c to
0d9726c
Compare
1 participant
Follow-up of #1115